8371915: [lworld] LayoutKind enum should have helper methods #1745
Conversation
|
👋 Welcome back fparain! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| props = (ArrayKlass::ArrayProperties)(ArrayKlass::ArrayProperties::NULL_RESTRICTED | ArrayKlass::ArrayProperties::NON_ATOMIC); | ||
| break; | ||
| case LayoutKind::NULLABLE_ATOMIC_FLAT: | ||
| props = ArrayKlass::ArrayProperties::NON_ATOMIC; |
There was a problem hiding this comment.
Why does NULLABLE_ATOMIC_FLAT produce NON_ATOMIC? Something, maybe just naming?, seems off here.
I know this is a refactoring of existing code and so has the same meaning but it doesn't make sense to me
There was a problem hiding this comment.
Good catch!
I've copied/pasted this code when refactoring without verifying it.
It doesn't make sense to me either.
For LayoutKind::NULLABLE_ATOMIC_FLAT the method should return ArrayKlass::ArrayProperties::DEFAULT.
Let me fix this and run some tests.
There was a problem hiding this comment.
No issue detected by testing.
| public: | ||
| enum ArrayProperties : uint32_t { | ||
| DEFAULT = 0, | ||
| DEFAULT = 0, // NULLABLE and ATOMIC |
There was a problem hiding this comment.
Does DEFAULT also apply to arrays of oops? The name "DEFAULT" is slightly concerning if we're only talking about non-oop cases. I'd be tempted to call this NULLABLE_ATOMIC rather than DEFAULT
There was a problem hiding this comment.
An array of oops (non-flat array) can be either DEFAULT or NULL_RESTRICTED.
DEFAULT is far from a perfect name, but it tries to reflect the fact that it is the semantic properties of arrays users get by default when creating an array, without using any special array creation API.
| UNKNOWN = 5 // used for uninitialized fields of type LayoutKind | ||
| }; | ||
|
|
||
| class LayoutKindHelper : AllStatic { |
There was a problem hiding this comment.
I wonder if this would be nicer if LayoutKind was a class and these helpers were in that class and encapsulate the whole thing in LayoutKind, so the accessors could be lk->is_flat(), etc? less characters. The enum inside LayoutKind could be referenced as LayoutKind::Kind::REFERENCE if direct references to the value is needed. Other enum class types in the VM are used that way (like AccessFlags). Then also it wouldn't need a helper.
There was a problem hiding this comment.
It might not be a good idea based on all the places that we read the LayoutKind enum values, in which case, never mind.
| } | ||
| return props; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a good refactoring. nit: I think the coding style has the break indented with the props = lines.
3 participants
Add some helper methods to test properties of layouts in a more reliable way.
Tested with Mach5, tier 1 to 3.
Fred
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1745/head:pull/1745$ git checkout pull/1745Update a local copy of the PR:
$ git checkout pull/1745$ git pull https://git.openjdk.org/valhalla.git pull/1745/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1745View PR using the GUI difftool:
$ git pr show -t 1745Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1745.diff
Using Webrev
Link to Webrev Comment